Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display warning when Nakadi SQL URL is not specified. #107

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

dlippok
Copy link
Collaborator

@dlippok dlippok commented Jun 13, 2022

One-line summary

Display warning message when a dump message is used but Nakadi SQL URL is not specified.

Description

Clin supports dumping both event definitions and Nakadi SQL definitions via common dump command. Nakadi SQL endpoint is separated from the messaging API and currently if the Nakadi SQL endpoint is not specified in the configuration Clin silently skips querying the SQL endpoint and goes directly to the messaging endpoint. This is misinterpreted by Nakadi backend and the returned result is an event definition instead of the SQL definition.

This behavior is misleading to the user of Clin that expects an SQL definition to be returned. In order to make the usage of Clin more interactive and friendly to the user, it will display a warning log line Configuration key nakadi_sql_url was not defined for your environment {env}. You won't be able to dump Nakadi SQL. when dump command is executed and the URL of the SQL endpoint was not configured.

Additionally nakadi_sql_url was added to the example configuration in the docs to make it clear that the configuration key is expected.

Types of Changes

  • Refactor/improvements
  • Documentation

Tasks

List of tasks you will do to complete the PR

None

Review

List of tasks the reviewer must do to review the PR

None

Deployment Notes

None

@dlippok dlippok changed the title Validate environment Display warning when Nakadi SQL URL is not specified. Jun 13, 2022
@Dmitry-Erokhin
Copy link
Collaborator

Hmmm, looks like with this change all application logging will land stderr. Have you considered something like sys.stderr.write("...") instead?

@dlippok
Copy link
Collaborator Author

dlippok commented Jul 18, 2022

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants